[editable-layers] Update to turf 7 and use geojson types#447
[editable-layers] Update to turf 7 and use geojson types#447charlieforward9 merged 16 commits intovisgl:masterfrom
Conversation
e536e8e to
b66ae77
Compare
There was a problem hiding this comment.
A hefty lift! Thank you for this. If we can resolve these points below, I think we can have this merged in by today.
Great to see all the @ts-expect-errors go away.
Im seeing a TON of FeatureCollection<SingleGeometry> instances, this would be better as a shorthanded, single export util coming from the same file these types are defined. Cleans up dependent files with a single export rather than two. Open to just about any name that is a shorthand of this.
Can you explain the removal of all the files in modules/editable-layers/src/lib/ and the curve-utils? Are they not used anywhere else across the repo or exported? (cc. @ibgreen, @georgioskarnas)
Also seeing a test fail from the geometry refactor.
From the original PR (I should copy the description across ):
A bunch of stuff from lib was exported (see the removals in |
b630e1c to
259afe0
Compare
Swapped for |
|
Excellent. Thank you for your time. Gonna pull a senior collaborator in here for a final check, and once they confirm, we'll see this finally land. |
ibgreen
left a comment
There was a problem hiding this comment.
Overall looks very good, thanks for putting this together.
Please consider the type renaming proposals and let me know.
modules/editable-layers/src/edit-modes/immutable-feature-collection.ts
Outdated
Show resolved
Hide resolved
ibgreen
left a comment
There was a problem hiding this comment.
Also I would like to see one line TSDoc comments for all the new types. These show up in vscode and makes it easier for users.
- A note in what's new that we are now using turf 7 would be nice.
- A doc page for geojson types is not P0 but wouldn't be unreasonable, mentioning that we are using
@geojson/typeswith extensions. - Also may want to make sure we are using latest
@types/geojson(perhaps that is already the case)
/** Simple geometries (excludes GeometryCollection) *?48a58e0 to
1d62275
Compare
|
I think I've addressed all the comments above. There's a lot of noise in the commit history now, so I'd suggest we squash. |
|
Incredible work. Squash and merge is default. Thanks for getting to this so quickly. |
Rebased and updated #221